Moved logging of incoming messages before processing#3703
Moved logging of incoming messages before processing#3703meee1 merged 2 commits intoArduPilot:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #3702 by ensuring incoming MAVLink packets are written to the telemetry log (.tlog) before any downstream processing that may synchronously send response packets (e.g., TIMESYNC), preventing replies from being logged earlier than the triggering request.
Changes:
- Moved incoming
SaveToTlog(...)call to occur immediately after packet verification and before message processing/handlers. - Renamed
getInfoFromStreamtoprocessInfoFromStreamto better reflect side effects (processing + possible responses).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // log before any processing that could send response packets (e.g. timesync) | ||
| try | ||
| { | ||
| SaveToTlog(new Span<byte>(buffer)); | ||
|
|
||
| if (logfile != null) | ||
| { | ||
| lock (logfile) |
There was a problem hiding this comment.
SaveToTlog() performs synchronous file I/O (and allocates via packet.ToArray() inside SaveToTlog) and is now executed while readlock is held. This extends the critical section that blocks other readPacketAsync() callers and can increase receive latency / risk backlog when logging to slow storage. Consider releasing readlock immediately after the packet is verified/resized, then perform tlog write + subsequent processing (including timesync/mission handling) outside the lock so reading can continue while logging/processing occurs.
There was a problem hiding this comment.
comment on 4926 says Readlock ended here, but it is ended at 5216. Seems like a serious consideration, the exact ordering of logging/processing/packet sequence error check is beyond my scope. Maybe the solution would be to move processInfoFromStream (or parts of it) down, instead of moving SaveToTlog up
…since it does more than just get
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
see issue #3702
Fix moves the
SaveToTlogmethod call beforegetInfoFromStream, which sends replies to TIMESYNC, MISSION_REQUEST_INT, etc. So not really just a getter, therefore renamed toprocessInfoFromStreamFirst time contributing, so open to all comments!